Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open RouteController to custom location snapping #85

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

sotomski
Copy link
Contributor

Description

This PR opens the RouteController.snappedLocation to be additionally modified by delegate implementers.
This is useful to plug in a custom snapping logic.

Infos for Reviewer

The intention was to open up the snapping logic but make it optional and don't change snapping behavior if custom logic is not provided. If custom snapping doesn't provide a valid value (ie. fails), the default snapping shall be used as fallback.

@@ -230,7 +230,13 @@ open class RouteController: NSObject, Router {
- important: If the rawLocation is outside of the route snapping tolerances, this value is nil.
*/
var snappedLocation: CLLocation? {
self.rawLocation?.snapped(to: self.routeProgress.currentLegProgress)
let snappedDefault = self.rawLocation?.snapped(to: self.routeProgress.currentLegProgress)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you doing duplicated work here? If there is a delegate, the delegate should do the snapping, otherwise the default snapping happens.

Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the nice PR, I think this will be helpful in using the library. Could you double check that we don't have to snap the location twice?

@sotomski sotomski changed the title feat: Open RouteController to custom location snapping Open RouteController to custom location snapping Jul 17, 2024
Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 - could you add a changelog entry for your changes?

@sotomski
Copy link
Contributor Author

Sure thing 👍

@boldtrn boldtrn merged commit 0d439d2 into maplibre:main Jul 18, 2024
2 checks passed
@boldtrn boldtrn mentioned this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants